Skip to content

Conversation

RoyEJohnson
Copy link
Contributor

@RoyEJohnson RoyEJohnson commented Sep 29, 2025

CORE-1264
The first installment of porting layouts/default to TypeScript, covering
footer
header.tsx
header/menus/menus.tsx
header/menus/give-button
header/menus/logo
header/menus/menu-expander
header/menus/upper-menu
header/sticky-note
lower-sticky-note
shared (coverage not quite complete here)

@RoyEJohnson RoyEJohnson force-pushed the core-1264-port-default-layout-part-1 branch 2 times, most recently from 76c81bc to bf04c06 Compare September 29, 2025 20:23
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the Welcome code, which was for use with my_openstax, which has been mothballed.

return (
<div className="page-header">
<JITLoad importFn={() => import('./sticky-note/sticky-note.js')} stickyData={stickyData} />
<StickyNote stickyData={stickyData} />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this load dynamically was not worth the hassle.

import PutAway from '~/components/put-away/put-away';
import './shared.scss';

// Shim for incognito windows that disable localStorage
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to main.js

await screen.findByText('What is your question about?');
expect(screen.queryAllByRole('navigation')).toHaveLength(0);
});
it('handles piTracker ', async () => {
Copy link
Contributor Author

@RoyEJohnson RoyEJohnson Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails regularly, and it is tested in user-link-handler.test anyway, so no need to do it here.

expect(button.parentElement?.classList.contains('active')).toBe(true);
await user.keyboard('{Escape}');
expect(button.parentElement?.classList.contains('active')).toBe(false);
// If clicked when not active, it does nothing
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is tested in the layouts/default test now.

@RoyEJohnson RoyEJohnson force-pushed the core-1264-port-default-layout-part-1 branch 2 times, most recently from 7e1e538 to 5bda6f8 Compare September 29, 2025 20:57
@RoyEJohnson RoyEJohnson force-pushed the core-1264-port-default-layout-part-1 branch 3 times, most recently from 5c36bd1 to 9850b0d Compare October 6, 2025 21:26
@RoyEJohnson RoyEJohnson force-pushed the core-1264-port-default-layout-part-1 branch from 27387cc to 05ae3c4 Compare October 8, 2025 15:05
@RoyEJohnson RoyEJohnson force-pushed the core-1264-port-default-layout-part-1 branch from 05ae3c4 to f820044 Compare October 8, 2025 19:13
import type {ArticleData} from '~/pages/blog/article/article';
import userEvent from '@testing-library/user-event';

// @ts-expect-error does not exist on
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had several of these routerFuture settings leftover.

@RoyEJohnson RoyEJohnson force-pushed the core-1264-port-default-layout-part-1 branch from f820044 to 7e06c2b Compare October 8, 2025 19:18
@RoyEJohnson RoyEJohnson requested a review from jivey October 8, 2025 19:19
Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good besides one concern about the localstorage shim.

src/app/main.js Outdated
clear() {},
key() {return null;},
length: 0
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably very edge case, but this doesn't seem to work if I disable localstorage in chrome. Maybe we should add a proxy? At the least, we can wrap it in a try to prevent the set property error (I only tested chrome, not sure if firefox and safari behave differently.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I examined the code to ensure that it is set up to handle missing/restricted localStorage, and removed the shim.

@RoyEJohnson RoyEJohnson requested a review from jivey October 14, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants